Conversation
jklein24
commented
Jan 27, 2026
- Add CLI package with TypeScript configuration
- Add config module for credential loading (~/.grid-credentials or env vars)
- Add HTTP client with Basic Auth support
- Add JSON output formatting utilities
- Add minimal CLI entry point with Commander.js setup
- Update project .gitignore, Makefile, and package.json with CLI targets
This stack of pull requests is managed by Graphite. Learn more about stacking. |
- Add CLI package with TypeScript configuration - Add config module for credential loading (~/.grid-credentials or env vars) - Add HTTP client with Basic Auth support - Add JSON output formatting utilities - Add minimal CLI entry point with Commander.js setup - Update project .gitignore, Makefile, and package.json with CLI targets
6bbe4ee to
9ee8dd9
Compare
Greptile OverviewGreptile SummaryThis PR adds a new Key issues to address are around request URL construction (base URL + relative path) and ensuring credential file permissions are actually tightened when overwriting existing files. Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| .gitignore | Adds ignores for CLI credentials and cli/dist output. |
| Makefile | Adds make targets for installing/building/running the new CLI; fixes missing newline. |
| cli/package.json | Introduces standalone CLI package with commander, TypeScript build, and Node>=18 engine. |
| cli/src/client.ts | Adds fetch-based HTTP client with Basic Auth; watch out for URL joining semantics if baseUrl lacks trailing slash. |
| cli/src/config.ts | Adds config loading from env or JSON file and saves credentials; consider ensuring credential file permissions are tightened on overwrite. |
| cli/src/index.ts | Adds commander program entrypoint and exports helpers; parsing on import can cause side effects for consumers. |
| cli/src/output.ts | Adds JSON output formatting helpers and exit code handling on error responses. |
| cli/tsconfig.json | Adds TS config for CLI build output; may need DOM lib for fetch types (already noted in prior thread). |
| package.json | Adds root npm scripts to install/build/run the CLI package. |
Sequence Diagram
sequenceDiagram
participant User as User
participant CLI as grid CLI (commander)
participant Config as config.ts
participant FS as ~/.grid-credentials (fs)
participant Env as process.env
participant Client as GridClient
participant API as Grid API
User->>CLI: run `grid ...` (options)
CLI->>Config: loadConfig({configPath, baseUrl})
alt configPath provided
Config->>FS: readFileSync(configPath)
FS-->>Config: JSON string
Config-->>CLI: GridConfig
else no configPath
Config->>Env: read GRID_* vars
Config->>FS: readFileSync(~/.grid-credentials) if exists
FS-->>Config: JSON string
Config-->>CLI: GridConfig
end
CLI->>Client: new GridClient(config)
CLI->>Client: request(method, path, {params, body})
Client->>API: fetch(url, headers: Basic Auth)
API-->>Client: JSON or text response
Client-->>CLI: ApiResponse{success,data|error}
CLI-->>User: output JSON (output.ts)
opt error
CLI-->>User: sets process.exitCode = 1
end
Greptile Review ResponseComment 1 (cli/src/config.ts:22) - JSON.parse for credentialsApplied. Added try-catch around JSON.parse with a clear error message for malformed credentials files. Comment 2 (cli/src/config.ts:38) - JSON.parse for configApplied. Added try-catch around JSON.parse with a clear error message for malformed config files. Comment 3 (cli/src/config.ts:69) - JSON.parse for save credentialsApplied. Added try-catch that starts fresh with an empty object if the existing credentials file is malformed. Comment 4 (cli/tsconfig.json:5) - Add DOM to libNot applicable. Node.js 18+ has native fetch and Comment 5 (cli/package.json:25) - Add engines fieldApplied. Added engines field specifying Node.js >=18.0.0 to document the native fetch requirement. Review response generated using |
- Add try-catch error handling for JSON.parse in config loading - Add engines field specifying Node.js >=18.0.0 requirement Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| path: string, | ||
| params?: Record<string, string | number | boolean | undefined> | ||
| ): string { | ||
| const url = new URL(path, this.config.baseUrl); |
There was a problem hiding this comment.
new URL(path, this.config.baseUrl) will drop the last path segment of baseUrl when baseUrl doesn’t end with / and path is relative (e.g., baseUrl https://api.lightspark.com/grid/2025-10-13 + path customers becomes https://api.lightspark.com/grid/customers). This will cause requests to hit the wrong endpoint unless callers always pass an absolute /... path or baseUrl always includes a trailing slash.
Also affects request() calls via buildUrl().
Consider normalizing baseUrl to include a trailing / (or always prefix path with /) before constructing the URL.
Context Used: Context from dashboard - CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/client.ts
Line: 37:37
Comment:
`new URL(path, this.config.baseUrl)` will drop the last path segment of `baseUrl` when `baseUrl` doesn’t end with `/` and `path` is relative (e.g., baseUrl `https://api.lightspark.com/grid/2025-10-13` + path `customers` becomes `https://api.lightspark.com/grid/customers`). This will cause requests to hit the wrong endpoint unless callers always pass an absolute `/...` path or `baseUrl` always includes a trailing slash.
Also affects `request()` calls via `buildUrl()`.
Consider normalizing `baseUrl` to include a trailing `/` (or always prefix `path` with `/`) before constructing the URL.
**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=21abe025-35ab-4ae8-a4a1-0071c2ac3b98))
How can I resolve this? If you propose a fix, please make it concise.| fs.writeFileSync(credentialsPath, JSON.stringify(merged, null, 2), { | ||
| mode: 0o600, | ||
| }); |
There was a problem hiding this comment.
fs.writeFileSync(..., { mode: 0o600 }) only reliably sets permissions on file creation; if the file already exists with broader permissions, Node won’t necessarily tighten them. This can leave credentials readable by other users depending on the prior mode.
If you want to guarantee 0600, you likely need to chmod after writing when the file already exists.
Context Used: Context from dashboard - CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/config.ts
Line: 85:87
Comment:
`fs.writeFileSync(..., { mode: 0o600 })` only reliably sets permissions on file creation; if the file already exists with broader permissions, Node won’t necessarily tighten them. This can leave credentials readable by other users depending on the prior mode.
If you want to guarantee `0600`, you likely need to `chmod` after writing when the file already exists.
**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=21abe025-35ab-4ae8-a4a1-0071c2ac3b98))
How can I resolve this? If you propose a fix, please make it concise.| export { program, getClient, GridClient, GridConfig }; | ||
|
|
||
| program.parse(process.argv); |
There was a problem hiding this comment.
program.parse(process.argv) runs on import, which makes this module hard to use as a library (and can surprise tests/other code that imports program or GridClient). This is especially relevant since you also export { program, getClient, ... }.
If you intend index.ts to be both the CLI entrypoint and an importable module, consider moving program.parse(...) behind a require.main === module / import.meta guard or into a separate bin entry file.
Context Used: Context from dashboard - CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/index.ts
Line: 40:42
Comment:
`program.parse(process.argv)` runs on import, which makes this module hard to use as a library (and can surprise tests/other code that imports `program` or `GridClient`). This is especially relevant since you also `export { program, getClient, ... }`.
If you intend `index.ts` to be both the CLI entrypoint and an importable module, consider moving `program.parse(...)` behind a `require.main === module` / `import.meta` guard or into a separate `bin` entry file.
**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=21abe025-35ab-4ae8-a4a1-0071c2ac3b98))
How can I resolve this? If you propose a fix, please make it concise.